-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
g0.135 Resolve problems and bring code to standard #136
Conversation
trpcultivate_phenotypes/src/Service/TripalCultivatePhenotypesGenusOntologyService.php
Outdated
Show resolved
Hide resolved
trpcultivate_phenotypes/src/Service/TripalCultivatePhenotypesGenusOntologyService.php
Outdated
Show resolved
Hide resolved
trpcultivate_phenotypes/src/Service/TripalCultivatePhenotypesTermsService.php
Outdated
Show resolved
Hide resolved
trpcultivate_phenotypes/src/Service/TripalCultivatePhenotypesTraitsService.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed the classes so far and made a couple of suggestions. I should get to reviewing tests on Friday. Overall, it's looking very polished!
Classes:
- src/Controller/TripalCultivatePhenotypesSettings.php
- src/Form/TripalCultivatePhenotypesOntologySettingsForm.php
- src/Form/TripalCultivatePhenotypesRSettingsForm.php
- src/Form/TripalCultivatePhenotypesWatermarkSettingsForm.php
- src/Service/TripalCultivatePhenotypesGenusOntologyService.php
- src/Service/TripalCultivatePhenotypesGenusProjectService.php
- src/Service/TripalCultivatePhenotypesTermsService.php
- src/Service/TripalCultivatePhenotypesTraitsService.php
trpcultivate_phenotypes/src/Service/TripalCultivatePhenotypesTraitsService.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove ConfigWatermarkFormTest::testBuildForm() and testValidateForm()?
trpcultivate_phenotypes/src/Controller/TripalCultivatePhenotypesSettingsController.php
Outdated
Show resolved
Hide resolved
trpcultivate_phenotypes/src/Service/TripalCultivatePhenotypesGenusOntologyService.php
Outdated
Show resolved
Hide resolved
trpcultivate_phenotypes/src/Service/TripalCultivatePhenotypesGenusOntologyService.php
Outdated
Show resolved
Hide resolved
trpcultivate_phenotypes/src/Service/TripalCultivatePhenotypesTermsService.php
Outdated
Show resolved
Hide resolved
trpcultivate_phenotypes/tests/src/Functional/ConfigWatermarkTest.php
Outdated
Show resolved
Hide resolved
trpcultivate_phenotypes/tests/src/Traits/PhenotypeImporterTestTrait.php
Outdated
Show resolved
Hide resolved
trpcultivate_phenotypes/tests/src/Unit/ConfigRRulesFormTest.php
Outdated
Show resolved
Hide resolved
trpcultivate_phenotypes/tests/src/Unit/ConfigWatermarkFormTest.php
Outdated
Show resolved
Hide resolved
I could not make this two test methods to pass since I updated the form controller to use DI 🙈 This is a unit test that I would like to replace with a kernel test. |
…esSettingsController.php Co-authored-by: Lacey-Anne Sanderson <[email protected]>
…enusOntologyService.php Co-authored-by: Lacey-Anne Sanderson <[email protected]>
…enusOntologyService.php Co-authored-by: Lacey-Anne Sanderson <[email protected]>
…ermsService.php Co-authored-by: Lacey-Anne Sanderson <[email protected]>
…est.php Co-authored-by: Lacey-Anne Sanderson <[email protected]>
…st.php Co-authored-by: Lacey-Anne Sanderson <[email protected]>
Co-authored-by: Lacey-Anne Sanderson <[email protected]>
…Trait.php Co-authored-by: Lacey-Anne Sanderson <[email protected]>
Co-authored-by: Lacey-Anne Sanderson <[email protected]>
….php Co-authored-by: Lacey-Anne Sanderson <[email protected]>
Co-authored-by: Lacey-Anne Sanderson <[email protected]>
…enusOntologyService.php Co-authored-by: Carolyn T Caron <[email protected]>
trpcultivate_phenotypes/tests/src/Unit/ConfigWatermarkFormTest.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks good to me now. All my previous suggestions were addressed ✅
Why did you remove ConfigWatermarkFormTest::testBuildForm() and testValidateForm()?
Not only was this test restored but we now have both unit and kernel for build and validate. Since the unit tests are blazingly fast (apparently 00:00.124 which felt instant to me), I think it's fine to have the overlap. The new tests accomplish the same thing/coverage the original ones did with added messages to the asserts.
There is one unit test for the submit that has no asserts that should be removed (see suggestion). Since this is covered in the new kernel tests, I think it is fine to delete it from the unit tests.
….php Co-authored-by: Lacey-Anne Sanderson <[email protected]>
…thub.com/TripalCultivate/TripalCultivate-Phenotypes into g0.135-resolveProblemsAndBringToStandard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything from me has been addressed! ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay! Looks ready to merge!
**Issue #135 ** Resolve Problems and bring up code to standard.
Motivation
What does this PR do?
Please describe each things this PR does. For example, a PR may 1) solve a specific bug, 2) create an automated test to ensure it doesn't return.
Testing
Automated Testing
Please describe each automated test this PR creates and provide a list of the assertions it makes using casual language.
Do not just say things like "asserts the array is not empty" but rather say "Ensures that the return value of method X with these parameters is not an empty array".
The following tests have been revised
Manual Testing
Describe in detail how someone should manually test this functionality.
Make sure to include whether they need to build a docker from scratch, create any records, etc.